[57932] Make Mails::MailerJob retry failed emails
#16871
Conversation
We want `MailerJob` to behave like both our `ApplicationJob` (for reloading the mailer configuration and resetting the request store automatically) and `ActionMailer::MailDeliveryJob` (for the basic functionalities of sending emails). As we can only inherit from one of these two classes, we inherit from `ApplicationJob` and copy the relevant parts from `ActionMailer::MailDeliveryJob`. This copy was done some time ago, and the `ActionMailer::MailDeliveryJob` class has evolved since then. This commit updates our copy to match the current `ActionMailer::MailDeliveryJob` class.
https://community.openproject.org/wp/57932 Retrying 14 times with polynomial backoff means it will retry for roughly 1.5 days. Previously the `retry_on StandardError` defined in the `Mails::MailerJob` class was ignored because there is also a `rescue_from StandardError` declared after it. As `retry_on` is implemented using `rescue_from`, and the handlers are evaluated in reverse order, the last declared `rescue_from` would be picked up and the retry logic would not be triggered. That's why mail jobs were discarded instead of being retried. This commit fixes the issue by inheriting directly from `ActionMailer::MailDeliveryJob` instead of redefining its methods. This was the `rescue_from` is declared first in the parent class, and then `retry_on` is declared second in the child class, meaning it will take precedence and be picked up. The needed shared logic from `ApplicationJob` is extracted to a new `SharedJobSetup` module and included in both `ApplicationJob` and `Mails::MailerJob`.
There was a problem hiding this comment.
Thanks a lot for fixing that, @cbliard ! While it doesn't make sense to test rails ActiveJob logic, we have clearly been using it wrong in the past and didn't notice that it didn't work as intended. This is why I think this justifies a spec to test that we are using it correctly.
Could you please add one?
|
Sure, I can try adding one. I'm curious to see what I will end up with. |
|
@machisuji I added a test for it. Can you review it again? Also I'm concerned that it will be a bit harder to find errors with unit tests: if a mailer produces a You can easily see it if you comment the line |
Great! Thanks for adding the test! As for your concern, I suppose it does obscure the failure for a test in case we do break a mailer somewhere. But usually it should still be found because we would probably test that a mail did go out and contained certain contents? I suppose you could amend the spec helper with a |
I get the idea but that would not work: Well, the jobs are not performed that often. It should not be too much of a problem. |
Ticket
https://community.openproject.org/wp/57932
What are you trying to accomplish?
Make OpenProject retry sending emails when sending fails for a temporary reason (short outage of provider for instance).
What approach did you choose and why?
Mailer jobs were discarded instead of being retried. Here is why:
Previously the
retry_on StandardErrordefined in theMails::MailerJobclass was ignored because there is also arescue_from StandardErrordeclared after it. Asretry_onis implemented usingrescue_from, and the handlers are evaluated in reverse order, the last declaredrescue_fromwould be picked up and the retry logic would not be triggered. That's why mail jobs were discarded instead of being retried.Here is how it was fixed:
The issue was fixed by inheriting directly from
ActionMailer::MailDeliveryJobinstead of redefining its methods. Thisway the
rescue_fromis declared first in the parent class, and thenretry_onis declared second in the child class, meaning it will take precedence (because declared last) and be picked up.The needed shared logic from
ApplicationJobis extracted to a newSharedJobSetupmodule and included in bothApplicationJobandMails::MailerJob.Merge checklist